-
Notifications
You must be signed in to change notification settings - Fork 16
Codejail Tutor plugin - MVP #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ejailservice and sandbox).
…tualenv is a binary an not a symlink (Required by AppArmor). Add sandbox user and install sudo, so sandbox code run without root permissions.
Add sandbox as codejail user.
Add codejail_apparmmor image to run an init job that creates the AppArmor profile on the host. Add lms patches to set additional settings. Update/fix codejail service settings. Set codejail processes limits to 0 (disable) to avoid errors while running jailed code.
| /sandbox/venv/bin/python Cx -> child, | ||
| profile child flags=(attach_disconnected,mediate_deleted){ | ||
| #include <abstractions/base> | ||
| #include <abstractions/python> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will only work if the same python version is installed on the host as in the container. I'm afraid we need to copy-paste the contents of this file into this template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@regisb This is the content of that file in my host:
/usr/lib{,32,64}/python{2.[4-7],3.[0-9]}/**.{pyc,so} mr,
/usr/lib{,32,64}/python{2.[4-7],3.[0-9]}/**.{egg,py,pth} r,
/usr/lib{,32,64}/python{2.[4-7],3.[0-9]}/{site,dist}-packages/ r,
/usr/lib{,32,64}/python3.[0-9]/lib-dynload/*.so mr,
/usr/local/lib{,32,64}/python{2.[4-7],3,3.[0-9]}/**.{pyc,so} mr,
/usr/local/lib{,32,64}/python{2.[4-7],3,3.[0-9]}/**.{egg,py,pth} r,
/usr/local/lib{,32,64}/python{2.[4-7],3,3.[0-9]}/{site,dist}-packages/ r,
/usr/local/lib{,32,64}/python3.[0-9]/lib-dynload/*.so mr,
# Site-wide configuration
/etc/python{2.[4-7],3.[0-9]}/** r,
# shared python paths
/usr/share/{pyshared,pycentral,python-support}/** r,
/{var,usr}/lib/{pyshared,pycentral,python-support}/** r,
/usr/lib/{pyshared,pycentral,python-support}/**.so mr,
/var/lib/{pyshared,pycentral,python-support}/**.pyc mr,
/usr/lib/python3/dist-packages/**.so mr,
# wx paths
/usr/lib/wx/python/*.pth r,
# python build configuration and headers
/usr/include/python{2.[4-7],3.[0-9]}*/pyconfig.h r,
I checked those paths in the codejailservice container and any of them actually exists, I think it is because I am using pyenv. Can we just then get rid of this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is that you cannot add #include <abstractions/python> to "docker-edx-sandbox" because you do not know which Python versions are installed on the host, and whether that file is even there. On my Ubuntu 20.04 computer, the /etc/apparmor.d/abstractions/python file is shipped with the apparmor Debian package and the library path regular expressions match Python version from 2.4 to 3.9. But on my Debian 4.9 server the /etc/apparmor.d/abstractions/python file is not even there. If it were there, it might not include support for Python 3.8.
So you need to copy-paste the contents of this file and to change the paths such that it matches the paths for the pyenv-installed python in the Docker container.
| WORKDIR /var/tmp | ||
| RUN mkdir -p common/lib/ | ||
| RUN svn export https://github.com/edx/edx-platform.git/tags/open-release/koa.3/common/lib/sandbox-packages common/lib/sandbox-packages | ||
| RUN svn export https://github.com/edx/edx-platform.git/tags/open-release/koa.3/common/lib/symmath common/lib/symmath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does svn export do? Do we really need subversion or can we get rid of it? If we just to copy a file from edx-platform, it would be safer to COPY it from the docker image labeled DOCKER_IMAGE_OPENEDX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, it is a good idea to copy it from the edx-platform image, I am going to try that.
| "limit_overrides": {}, | ||
| } | ||
|
|
||
| apply_django_settings(CODE_JAIL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm mistaken, creating a Django project would require defining a database. A full Django project seems overkill for what you are trying to achieve here, meaning a stateless python interpreter with a very basic API. Can I suggest relying on a more lightweight framework instead, such as Flask?
Get sandbox requirements form openedx edxapp image.
Add codejail service version as variable.
| WORKDIR /openedx/codejailservice | ||
|
|
||
| EXPOSE 8000 | ||
| CMD gunicorn --workers=2 --name codejailservice --bind=0.0.0.0:8000 --max-requests=1000 codejailservice.wsgi:application |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NeOneSoft please change here to use uwsgi instead of gunicorn
Fix codejail service default host.
| "DOCKER_IMAGE": f"docker.io/ednxops/codejailservice:{__version__}", | ||
| }, | ||
| "set":{ | ||
| "SANDBOX_PYTHON_VERSION": "3.5.10", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest moving this to the "defaults" section. Then, use it as CODEJAIL_SANDBOX_PYTHON_VERSION. When possible, we should really try to avoid "set" entries. They are only required to override existing tutor settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
tutorcodejail/plugin.py
Outdated
| hooks = { | ||
| "build-image": { | ||
| "codejail": "{{ CODEJAIL_DOCKER_IMAGE }}", | ||
| "codejail_apparmor": "docker.io/ednxops/codejail_apparmor:latest" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to replace ":latest" by the actual plugin version. This guarantees that the Docker image is always pinned. This is why I always add a "VERSION" setting. See for instance: https://github.com/overhangio/tutor-mfe/blob/932f2db9fdf4c9370abeec5e89a558d9f590f133/tutormfe/plugin.py#L13
| RUN mkdir -p common/lib/ | ||
|
|
||
| COPY --from={{ DOCKER_IMAGE_OPENEDX }} /openedx/edx-platform/common/lib/sandbox-packages common/lib/sandbox-packages | ||
| COPY --from={{ DOCKER_IMAGE_OPENEDX }} /openedx/edx-platform/common/lib/sandbox-packages common/lib/symmath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a copy-paste typo? Shouldn't it be:
COPY --from={{ DOCKER_IMAGE_OPENEDX }} /openedx/edx-platform/common/lib/symmath common/lib/symmath
?
| @@ -0,0 +1,87 @@ | |||
| FROM docker.io/ubuntu:20.04 as minimal | |||
| MAINTAINER Overhang.io <contact@overhang.io> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should change this if you intend to support this image :)
|
|
||
| ARG SANDBOX_PYTHON_VERSION={{ SANDBOX_PYTHON_VERSION }} | ||
| RUN $PYENV_ROOT/bin/pyenv install $SANDBOX_PYTHON_VERSION | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future, I think that all code below this line should be run as a different user (e.g: USER openedx). This is important for people running Docker containers in unprivileged environments (as is often the case in Kubernetes). It's not a deal-breaker for this first release, but you should this in mind for the future.
| codejailservice: | ||
| image: {{ CODEJAIL_DOCKER_IMAGE }} | ||
| environment: | ||
| FLASK_APP_SETTINGS: config.DevelopmentConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we want production settings here instead?
| FLASK_ENV: development | ||
| FLASK_APP_SETTINGS: codejailservice.config.DevelopmentConfig | ||
| ports: | ||
| - "8550:8550" No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add: restart: unless-stopped.
| @@ -0,0 +1,7 @@ | |||
| from base import BaseConfig | |||
|
|
|||
| class DevConfig(BaseConfig): | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation looks funky here.
|
I tested this today using an "august" version of edx-platform with the changes of https://github.com/edx/edx-platform/pull/27795 and it worked as a POC. I think we should merge this as is and iterate on improving the QA and documentation and fixes that come along as we find them. Having this in the PR is making it more complicated for people just looking at this now. |
| class DevConfig(BaseConfig): | ||
| pass | ||
|
|
||
| class ProductionConfig(BaseConfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Syntax missing a :
This is the first functional version of a tutor plugin that allows running Codejail using an additional REST API service that runs in a separate container than the edx-platform one.